oc: Use default resources where it makes sense #4947

0xmichalis · 2015-10-05T13:44:22Z

Commands I have defaulted using the builder/RESTMapper in this PR:

oc deploy
(Was using both syntaxes before, now achieves it via the resource builder.)
oadm build-chain
(Was using both syntaxes before, now achieves it via the RESTMapper.)
oc start-build
(Was using only NAME syntax, now uses bc/NAME too.)
oadm secrets add
(Was using only TYPE/NAME syntax, now uses NAME too.)
oc cancel-build
(Was using only NAME syntax, now uses build/NAME too.)

Fixes #4781
Fixes #1995

deads2k · 2015-10-06T13:00:39Z

pkg/cmd/cli/cmd/deploy.go

deads2k · 2015-10-06T13:02:49Z

This command is requiring resource/name format: https://github.com/openshift/origin/blob/master/pkg/cmd/cli/secrets/add_secret_to_obj.go

liggitt · 2015-10-06T13:21:43Z

This command is requiring resource/name format: https://github.com/openshift/origin/blob/master/pkg/cmd/cli/secrets/add_secret_to_obj.go

For two args, no less

0xmichalis · 2015-10-10T16:32:58Z

[test]

0xmichalis · 2015-10-10T18:31:03Z

@deads2k jenkins is green, merge at your will

liggitt · 2015-10-10T18:37:56Z

Are there tests to make sure prior invocations still work correctly?

0xmichalis · 2015-10-10T19:29:41Z

Are there tests to make sure prior invocations still work correctly?

test-cmd.sh should catch such errors.

liggitt · 2015-10-10T19:51:57Z

I agree it should, just not sure it does :)
Can you summarize what invocations were allowed prior (with/without types, abbreviations and full resource names), and what are allowed now?
For example, abbreviations like oc secrets add sa/sa-name ... were allowed, but I don't think test-cmd.sh exercises that

0xmichalis · 2015-10-11T14:17:07Z

Can you summarize what invocations were allowed prior (with/without types, abbreviations and full resource names), and what are allowed now?

I've updated the first post with more details about and I've also updated test-cmd to test all cases.

smarterclayton · 2015-10-11T15:00:30Z

pkg/cmd/cli/cmd/startbuild.go

This seems like something that should be handled by resource builder?

I am not using the resource builder here because it would try to get the buildConfig (which we are not interested in) in order to start the build. Instead, I think using the RESTMapper to resolve the resource type is enough.

It makes me believe that what you are representing here is a common pattern
that should be abstracted either into resource.Builder (but without the
fetch) or as supporting utility code in the cmd packages.

On Oct 11, 2015, at 3:38 PM, Michail Kargakis [email protected]
wrote:

In pkg/cmd/cli/cmd/startbuild.go
#4947 (comment):

name := buildName
isBuild := true

if len(name) == 0 {

name = args[0]

if len(name) == 0 && len(args[0]) > 0 {

parts := strings.Split(args[0], "/")

I am not using the resource builder here because it would try to get the
buildConfig (which we are not interested in) in order to start the build.
Instead, I think using the RESTMapper to resolve the resource type is
enough.

—
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/4947/files#r41714535.

Added an utility under origin/pkg/cmd/util to handle this.

0xmichalis · 2015-10-12T16:32:21Z

re[test]

0xmichalis · 2015-10-12T18:47:18Z

go stdlib race

rere[test]

0xmichalis · 2015-10-14T16:42:04Z

Tagging more eyes:

@ironcladlou about deploy
@mfojtik about {start,cancel}-build

deads2k · 2015-10-14T18:07:20Z

pkg/cmd/experimental/buildchain/buildchain.go

I think what you really want is a way to get a canonical resource representation. Something where you get "imagestreamtag" back for "istag", "istags", "imagestreamtags" in any case. If multiple resources deal in the same kind, this breaks because "notAnIsTag/foo" would match.

I asked @smarterclayton if he knew whether it existed, he said he'd look for it/make it.

0xmichalis · 2015-10-22T09:40:06Z

[test]

smarterclayton · 2015-10-27T03:12:13Z

[test]

smarterclayton · 2015-10-27T03:14:19Z

Test flake in #5410

On Sat, Oct 24, 2015 at 6:00 PM, OpenShift Bot [email protected]
wrote:

Origin Action Required: Pull request cannot be automatically merged,
please rebase your branch from latest HEAD and push again

—
Reply to this email directly or view it on GitHub
#4947 (comment).

liggitt · 2015-10-27T12:13:04Z

pkg/cmd/cli/cmd/startbuild.go

don't you need to make sure you got back a non-empty name? I would do so after this block, to catch the case where len(args[0]) == 0

liggitt · 2015-10-27T20:32:45Z

one question on fromRepo, fromFile, etc interaction from @smarterclayton, then lgtm

smarterclayton · 2015-10-27T22:10:32Z

if len(args) > 0 || len(buildName) > 0 || len(fromFile) > 0 || len(fromDir) > 0 || len(fromRepo) > 0 {

we're safe.

On Tue, Oct 27, 2015 at 4:32 PM, Jordan Liggitt [email protected]
wrote:

one question on fromRepo, fromFile, etc interaction from @smarterclayton
https://github.com/smarterclayton, then lgtm

—
Reply to this email directly or view it on GitHub
#4947 (comment).

liggitt · 2015-10-29T15:14:44Z

pkg/cmd/cli/cmd/startbuild.go

move this check outside the if block to catch cases where len(name)==0, but len(args[0]) == 0, for example

openshift-bot · 2015-10-30T10:24:53Z

Evaluated for origin test up to e30dccd

openshift-bot · 2015-10-30T11:22:33Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/6430/)

liggitt · 2015-10-31T03:13:09Z

LGTM, ready to merge?

0xmichalis · 2015-10-31T05:05:36Z

Lets do it

liggitt · 2015-10-31T05:10:42Z

[merge]

openshift-bot · 2015-10-31T05:24:50Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/3879/) (Image: devenv-rhel7_2614)

openshift-bot · 2015-10-31T05:24:50Z

Evaluated for origin merge up to e30dccd

Merged by openshift-bot

deads2k reviewed Oct 6, 2015
View reviewed changes

pkg/cmd/cli/cmd/deploy.go Outdated

Copy link

Contributor

deads2k Oct 6, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lowercase?

0xmichalis closed this Oct 10, 2015

0xmichalis reopened this Oct 10, 2015

smarterclayton reviewed Oct 11, 2015
View reviewed changes

deads2k reviewed Oct 14, 2015
View reviewed changes

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 22, 2015

liggitt reviewed Oct 27, 2015
View reviewed changes

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 29, 2015

liggitt reviewed Oct 29, 2015
View reviewed changes

oc: Use default resources where it makes sense

e30dccd

0xmichalis closed this Oct 30, 2015

0xmichalis reopened this Oct 30, 2015

openshift-bot pushed a commit that referenced this pull request Oct 31, 2015

Merge pull request #4947 from kargakis/default-resources-in-commands

9501700

Merged by openshift-bot

openshift-bot merged commit 9501700 into openshift:master Oct 31, 2015

0xmichalis deleted the default-resources-in-commands branch October 31, 2015 10:57

oc: Use default resources where it makes sense #4947

oc: Use default resources where it makes sense #4947

Uh oh!

Conversation

0xmichalis commented Oct 5, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented Oct 6, 2015

Uh oh!

liggitt commented Oct 6, 2015

Uh oh!

0xmichalis commented Oct 10, 2015

Uh oh!

0xmichalis commented Oct 10, 2015

Uh oh!

liggitt commented Oct 10, 2015

Uh oh!

0xmichalis commented Oct 10, 2015

Uh oh!

liggitt commented Oct 10, 2015

Uh oh!

0xmichalis commented Oct 11, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xmichalis commented Oct 12, 2015

Uh oh!

0xmichalis commented Oct 12, 2015

Uh oh!

0xmichalis commented Oct 14, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xmichalis commented Oct 22, 2015

Uh oh!

smarterclayton commented Oct 27, 2015

Uh oh!

smarterclayton commented Oct 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt commented Oct 27, 2015

Uh oh!

smarterclayton commented Oct 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-bot commented Oct 30, 2015

Uh oh!

openshift-bot commented Oct 30, 2015

Uh oh!

liggitt commented Oct 31, 2015

Uh oh!

0xmichalis commented Oct 31, 2015

Uh oh!

liggitt commented Oct 31, 2015

Uh oh!

openshift-bot commented Oct 31, 2015

Uh oh!

openshift-bot commented Oct 31, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants